Repository navigation
fix(#5852): declare sudo -a/-c and doas -a as value options in the secret-read guard - #5874
potterdigital wants to merge 4 commits into
Conversation
…n the secret-read guard The WRAPPERS table added in open-gsd#5685 left out three value-taking options that sudo(8) and doas(1) document: sudo's -a/--auth-type and -c/--login-class, and doas's -a. resolveCommand took the option's value as the command word, so `sudo -c ls cat .env` resolved to `ls` and was allowed, as was `sudo -c staff bash -c 'cat .env'`. 1.18.0 denied them. Adds the entries and a open-gsd#5852 regression block; the open-gsd#5627 wrapper list takes the new options so its property test covers them.
trek-e
left a comment
There was a problem hiding this comment.
Summary
This PR fixes #5852 by declaring sudo's -a/--auth-type and -c/--login-class, and doas's -a, as value-taking options in the secret-read guard's WRAPPERS table. The fix site and short-option set are right, and the tests cover every acceptance criterion. However, the new long entry --login-class opens a new bypass through the table's prefix matching: sudo --login cat .env was denied before this PR and is allowed after it. The required Self-test report check is also red. Changes are requested.
Classification & gate compliance
- #5852 is labelled
confirmed-bug, so this is the Bug-Fix Track. The PR body links it withFixes #5852. - The branch is
fix/5852-…and the commits are conventional. - The changeset
tidy-birds-tumble.mdisFixedwithpr: 5874. - The branch is behind
next. This repo merges through the merge queue, so that is not a finding. It is mergeable. - CI on 361c66f:
Self-test reportfails withfail_suite_not_passed(Blocker 1).Reviewed or maintaineris waiting on this review.- 9 fork runs are awaiting approval and have not been released, because this review has findings above Nit.
- There is no competing PR for #5852. #5649 edits the same
resolveCommandregion but not the table.
Root-cause verdict
The root cause is correct. On next, sudo's short set (CDghpRrTtUu) lacks a and c, and its long list lacks --auth-type and --login-class. doas's short set (Cu) lacks a. As a result:
wrapperOptionWidth(hooks/gsd-secret-read-guard.js:886) gives those options width 1.skipWrapperArgsstops at the option's value.resolveCommandreturns that value as the command word.findSecretReadthen skips the segment when that word is a non-reading command.
The table edit is the right fix site. I reproduced this by reading the code on base and head. I did not run the hook.
Blast radius
get_impactonresolveCommand(next declaration, upstream, depth 2): MEDIUM, 9 affected, all inhooks/gsd-secret-read-guard.js. Direct callers:reconstructedScript,findSecretRead,findDebuggerCliSpawnandscanXargsPipe. At depth 2:scanGitConfig,reconstructedProcsub,findSecretReadUnderOtherReadingsandscanShellInterpreter.- Consequence: every consumer that resolves a command word inherits the new widths, including the bug in Major 1.
Memtrace Evidence
- Index state: stale. The graph is at 894a8c0 and
nextis at 4dce9e8. Every line reference below was checked against the PR checkout. detect_changes(all 3 changed-file blocks):- The guard:
WRAPPERSat:165-177. - The test file: the new describe closures.
- The changeset: no symbols, because it is not indexed.
- The guard:
find_cross_module_issues(all 3 blocks): 0 on the tests and changeset blocks. On the guard block it returned 1 issue,inconsistent_type_consumptionon a generic.lengthinscripts/changeset/new.cjs:45. That is a false positive: the file is not in the diff, and the symbol is the built-inlengthproperty.find_code_review_issues(all 3 blocks,review_mode: online,max_candidates: 100): the same single false positive, otherwise 0.find_yaml_rule_matches(all 3 blocks): 0.find_ast_review_issues: not run, because the diff has no.pyfiles.review_github_pr(post: false): 0 comments.get_symbol_contextonWRAPPERS: a constant at:165-177. It has one reference, theWRAPPERS.getlookup inresolveCommand(:930), and belongs to communityAssertPathTest.get_impactonresolveCommand: MEDIUM, 9 affected, as above.recall_decision: returnedunavailable, because the MemCortex IPC circuit is open. The recorded-decision gate did not run.governing_rulesonhooks/gsd-secret-read-guard.js:governed: false.
Laws that apply
Subject: the guard's wrapper table now declares sudo -a/-c (and their long forms) and doas -a as value-taking.
- postels-law
- Fired:
WRAPPERSis a parser table. Any option it misclassifies fails open, and the long list is read with prefix matching (:890). - Action: new long entries must not prefix-collide with a no-value option (Major 1).
- Fired:
- kerckhoffs-principle
- Fired: the guard has to stay safe with its table public.
sudo --login cat .envis a public bypass created by this edit. - Action: fix Major 1 and pin it with a test.
- Fired: the guard has to stay safe with its table public.
- hyrums-law
- Cleared: the new denials restore 1.18.0's behaviour. The allow cases (
sudo -c staff ls .env,-n,-E,-H) are pinned.
- Cleared: the new denials restore 1.18.0's behaviour. The allow cases (
Considered and cleared:
kernighans-law: a two-line data change.knuths-optimization-principle: no performance change.leaky-abstractions: no new abstraction.
Findings
Blocker
-
The required
Self-test reportcheck is red. Your report recordsfull_suite.result: failed(48,602 passed, 4 failed). None of the failures are in files this PR touches:- 2 failures (
verify-work-auto-transition,ship-phase-complete-gate): the container has nojq. - 1 failure (
helpers-cleanup): the clone sat under/tmp. - 1 failure: the #5100 win32 install probe. That failure on WSL hosts is now filed as #5888.
This PR still needs a self-test report with a passing full suite before it can merge. Run it with
jqinstalled, from a clone outside/tmp, on a non-WSL host or after #5888 is fixed. - 2 failures (
Major
--login-classmakessudo --login cat .envread the secret. This comes from the isolated adversarial pass, and I checked it against the source.- Where:
wrapperOptionWidth(hooks/gsd-secret-read-guard.js:890) matches long options by prefix:(spec.long || []).some((l) => l.startsWith(word)). - How it fails: sudo's
--login(the long form of-i) takes no value. With--login-classadded to the long list,'--login-class'.startsWith('--login')is true, so--loginnow gets width 2. Insudo --login cat .env,catis consumed as its value and.envbecomes the command word, so the read is allowed. - On
next:--loginhas width 1,catis the command, and the command is denied. - Fix: a word that exactly names an option sudo documents as taking no value must not prefix-match a longer valued one. Either check an exact match against a no-value list first, or only prefix-match when the word is not itself a complete documented long option.
- Test: add
sudo --login cat .env(blocked). Also addsudo --login bash -c 'cat .env'(blocked), since this flag is in common use.
- Where:
Minor
- The new options are tested through only one shared consumer.
resolveCommandfeedsreconstructedScript,scanXargsPipeandfindDebuggerCliSpawn, but only the debugger-spawn check gets a dedicated test. Add onexargs-pipe case (for exampleecho .env | xargs sudo -c ls cat) so the other consumers are pinned too.
Considered and cleared
- Short-option completeness: after this PR, sudo has
-a -C -c -D -g -h -p -R -r -T -t -U -uand doas has-a -C -u. That matches the synopses quoted in #5852. The adversarial pass did not re-fetch the man pages. - Attached and clustered forms:
-cls,-nc lsand-Ec lsresolve correctly through the loop at:893-895.--opt=valuereturns width 1 (:888). --anow matching--auth-type: real sudo rejects--aas ambiguous (it also has--askpass), so nothing runs. No action.- Security pass and code-review pass: both clean. They did not test the
--logininteraction.
Verdict
Changes requested: 1 Blocker, 1 Major, 1 Minor. The table entries are right, but the long-option prefix match turns --login-class into a new bypass for sudo --login. Fix that and add a test for it, then provide a passing Self-test report.
…in-class wrapperOptionWidth prefix-matches long options, so adding the valued --login-class made sudo's no-value --login take the next word as its value: 'sudo --login cat .env' resolved to '.env' and was allowed. sudo's spec now lists its no-value long options (sudo(8)), and an exact match on one returns width 1 before prefix matching, as getopt_long prefers an exact match. Tests pin --login direct, nested-shell and xargs-upstream forms.
|
Pushed bfa3c49 for the review. Major 1 ( New tests: Minor 1 (xargs consumer). Blocker 1 (Self-test report). I ran the full suite on a non-WSL host: this repo's Tests workflow, dispatched on the fork at bfa3c49: https://github.com/potterdigital/gsd-core/actions/runs/41904702557. Every job is green, including the three full-scope ubuntu shards, the windows and macOS conformance lanes, the coverage gate and Required tests. Across the full ubuntu shards: 39,968 tests, 39,856 passed, 0 failed, 111 skipped, 1 todo. The PR body's self-test report is updated to bfa3c49, and |
trek-e
left a comment
There was a problem hiding this comment.
Summary
This re-review covers bfa3c49b, which answers my CHANGES_REQUESTED review of 2026-11-04. All three items are fixed:
- Major 1 (
sudo --login): sudo's entry now lists its no-value long options inflags.wrapperOptionWidthreturns width 1 for an exact match on one of them before prefix matching, which is getopt_long's own exact-match rule.sudo --login cat .envis denied again. It is pinned along withsudo --login bash -c 'cat .env',sudo --login -u root cat .env,sudo --login ls .env(allowed), and the xargs-upstream form. - Minor 1 (other consumers): you were right that
scanXargsPipetakes xargs's own sub-command word as is and resolves only the upstream segment (hooks/gsd-secret-read-guard.js,scanXargsPipe). The new upstream-wrapper rows pin the path that actually runs. - Blocker 1 (Self-test report): the report now records the full-scope Tests workflow green at this head, and
Self-test reportpasses.
The fix restores 1.18.0's verdicts for every command #5852 lists, and I found no new bypass.
Classification & gate compliance
- Linked issue:
Fixes #5852, OPEN and labelledconfirmed-bug, so this is on the Bug-Fix track. - Merge state: merges cleanly into
next. Being behindnextis not a finding, becausenextuses a merge queue. - Format: Conventional Commits, and a
Fixedchangeset withpr: 5874and a bold, symptom-led lead. - Competing PRs: none.
- CI: the fork runs were held at
action_required. I released them after the review found nothing above Nit, and watched them complete green on this head before posting this review.
Root-cause verdict
The root cause is confirmed against the #5852 triage. WRAPPERS (hooks/gsd-secret-read-guard.js:165-170) lacked sudo's -a/--auth-type and -c/--login-class and doas's -a. wrapperOptionWidth therefore gave those options width 1, and resolveCommand returned the option's value as the command word.
The table entries are the right fix site. The flags exact match closes the one prefix collision the new --login-class entry creates.
Anti-paper-over gate: passes.
Blast radius & risk tier
get_impactonwrapperOptionWidth(upstream, depth 3): MEDIUM, 6 affected, all inhooks/gsd-secret-read-guard.js. The chain isskipWrapperArgs→resolveCommand→reconstructedScript,findSecretRead,findDebuggerCliSpawnandscanXargsPipe.- Every consumer that resolves a command word inherits the corrected widths. The debugger-spawn and xargs consumers are pinned by the new tests.
- Risk tier: the changed symbols are not in the sweep's top-25 central or bridge symbols.
Co-change completeness
The hook and its test file change together, and the changeset type is Fixed, so no docs update is required. The triage note about the header's accepted-gaps wording still reads accurately after this change.
Memtrace Evidence
The graph is at 894a8c09e, behind next, so graph answers are lower bounds, and line references were checked against the PR checkout.
get_impact— targetwrapperOptionWidth(hooks/gsd-secret-read-guard.js): MEDIUM, 6 affected (skipWrapperArgs,resolveCommand,reconstructedScript,findSecretRead,findDebuggerCliSpawn,scanXargsPipe).get_symbol_context— targetwrapperOptionWidth: one caller,skipWrapperArgs(:912), communityAssertPathTest.recall_decision— query "secret-read guard wrapper option table sudo doas value options prefix matching": MemCortex unavailable (MemCortex local IPC deadline elapsed), so I could not check for a recorded decision.governing_ruleson the hook returnedgoverned: false.find_code_review_issues— run withreview_mode: onlineandmax_candidates: 100on all 3 changed file blocks. It raised one raw issue,inconsistent_type_consumptiononlengthdeclared insrc/fallow-runner.ctswith consumerscripts/changeset/new.cjs:45. That is not a defect: the diff touches neither file.detect_changes,find_cross_module_issuesandfind_yaml_rule_matches— run on all 3 blocks. The only hit was that samelengthitem.review_github_pr(post: false,reviewMode: online,minSeverity: low): 0 comments.- There are no
.pyfiles, sofind_ast_review_issuesdoes not apply.
Laws that apply
Subject: the secret-read guard's WRAPPERS table now declares sudo -a/-c (and their long forms) and doas -a as value-taking, and wrapperOptionWidth gives an exact no-value sudo long option width 1 before prefix matching.
- postels-law — fired:
WRAPPERSis a parser table, and an option it misclassifies fails open. Action: none. It is already honored: an exact match on a documented no-value option now wins over a longer valued option it prefixes, which is getopt_long's own rule. A unique abbreviation (--login-c) still prefix-matches. - kerckhoffs-principle — fired: the guard has to stay safe with its table public, and the first revision's
sudo --login cat .envbypass was a public one. Action: none. It is already honored: the bypass is closed and pinned. - hyrums-law — fired: the guard's verdicts change. Action: none. It is already honored: the new denials restore 1.18.0's behaviour, and the allow cases are pinned.
Considered and cleared:
- kernighans-law: one guarded early return plus table data.
- knuths-optimization-principle: no performance change.
- leaky-abstractions: no abstraction is introduced or bypassed.
Action items: none.
Findings
None.
Considered and cleared
- The abbreviations
--lo,--logand--logistill prefix-match--login-class, giving width 2. The isolated adversarial pass raised this. Each of them also prefixes--login, a no-value option with a differenthas_arg, so getopt_long reports the word as ambiguous and sudo exits without running anything. The hook's verdict on a command that never runs is not a bypass. This is the same reason my earlier review cleared--a. - No parity test between
longandflags, and no new fast-check property. The Standards and adversarial passes raised this. The two lists are complementary, not parallel copies:flagsexists to break the one prefix collision, and no valued long option equals a flag. The existing property "any wrapper stack in front of bash -c "cat .env" is blocked" draws its stack from thewrapperslist this PR extends ('sudo -c staff','sudo --login','doas -a passwd'and others), so the new entries are already property-covered. - The
=-attached and clustered forms (--login-class=staff,sudo -ic ls) are not pinned in the new describe. They go through the generic paths (:888returns width 1 for any--opt=value, and the cluster loop is shared by every wrapper) that this PR does not change. - The generic
spec.flagsbranch is populated only for sudo. It solves a concrete collision, and other wrappers fall through unchanged. /security-review: the change narrows what the guard allows, with no new input surface, and found nothing at confidence ≥ 8.
Verdict
Approved. CI was released and completed green on bfa3c49b before this review was posted.
|
@potterdigital this is approved, and it merges as soon as one thing is fixed. That thing comes from me, not from your change. To refresh a stale Please update the report to the current head |
|
@trek-e The self-test report in the PR body now names head
No code change. |
Fix PR
Linked Issue
Fixes #5852
What was broken
The secret-read guard's
WRAPPERStable (added in #5685) left out three value-taking options that sudo(8) and doas(1) document: sudo's-a/--auth-typeand-c/--login-class, and doas's-a. The guard took the option's value as the command word, sosudo -c ls cat .env,sudo --auth-type echo cat .env,sudo -c timeout cat .envanddoas -a ls cat .envwere allowed (1.18.0 denied them), and the debugger-spawn check readsudo -c ls claude --agent gsd-debuggeras anlscommand.What this fix does
Adds
aandcto sudo'sshortset,--auth-typeand--login-classto itslonglist, andato doas'sshortset. sudo's entry also lists its no-value long options from sudo(8) (flags), andwrapperOptionWidthgives an exact match on one of them width 1 before it tries prefix matching, the way getopt_long prefers an exact match. Without that, the new valued--login-classwould prefix-match sudo's no-value--login(-i), sosudo --login cat .envwould takecatas a value and resolve.envas the command word.resolveCommandand its callers are unchanged. The header's documented gap ("a wrapper option taking a separate value the WRAPPERS table does not list") still describes unlisted options accurately, so it is left as is.Root cause
wrapperOptionWidthgives an option not in the table a width of 1, soskipWrapperArgsstops at the option's value andresolveCommandreturns it as the command word. When that word is inNON_READING_COMMANDSthe segment is skipped, andtimeoutswallowscatas its DURATION.Testing
How I verified the fix
The issue's reproduction script, run against the hook on
next3730c67 and on this branch (Node 24.21, Linux):The
--loginrows (basenext3730c67, the first revision 361c66f, this branch bfa3c49):sudo -c staff ls .envflips to allow because-cnow consumesstaffand the command isls, which matches the verdict for a barels .env. Base also allowedsudo -c staff bash -c 'cat .env'(the nested-shell route failed open for any value, not only non-reading names); this branch blocks it.Regression test added?
tests/gsd-secret-read-guard.test.cjs: a new#5852block (the six commands from the issue, attached-value forms-cls/-als, the nested-shell route, the still-blocked controls, the allows that show the option consumes exactly one word and that-n/-E/-Hstill resolve to the next word, and the debugger-spawn case). The bug(hooks): the secret-read guard loses a nested shell behind a flagged or unlisted wrapper —timeout 5 bash -c 'cat .env',nice -n 5 bash -c …,env -i bash -c …are allowed #5627 wrapper list takessudo -c staff,sudo --login-class staff,sudo -a bsdauth,sudo --auth-type bsdauthanddoas -a passwd, so its property test stacks them too. It also pins sudo's no-value--login(sudo --login cat .env,sudo --login bash -c 'cat .env',sudo --login -u root cat .envblocked;sudo --login ls .envallowed;sudo --loginandsudo --login -u rootin the bug(hooks): the secret-read guard loses a nested shell behind a flagged or unlisted wrapper —timeout 5 bash -c 'cat .env',nice -n 5 bash -c …,env -i bash -c …are allowed #5627 wrapper list) and the xargs-pipe consumer, which runsresolveCommandon the upstream segment (sudo -c staff echo .env | xargs cat,sudo --login echo .env | xargs cat). On this branch the file passes 669/669. With thenexthook swapped in, 18 fail; with the first revision's hook (361c66f), 6 fail, all of them the--loginrows.Platforms tested
Runtimes tested
Self-test report
{ "schema": 1, "head_sha": "7fe59cf85d0b5c0626207618a83121747b74c66f", "tool": "npm-test", "failing_first": { "test": "tests/gsd-secret-read-guard.test.cjs", "fails_on_base": true, "passes_on_head": true }, "commands": [ "node --test tests/gsd-secret-read-guard.test.cjs", "npx eslint hooks/gsd-secret-read-guard.js tests/gsd-secret-read-guard.test.cjs", "Tests workflow (.github/workflows/test.yml) on this PR at this head: npm run test:coverage:unit:raw -- --shard 1/3, 2/3, 3/3, plus test:integration, test:security, test:install, test:slow" ], "full_suite": { "command": "Tests workflow on this PR at 7fe59cf8, full-scope lanes test (ubuntu-latest, 24, shard 1/3..3/3): https://github.com/open-gsd/gsd-core/actions/runs/42143668096", "result": "passed", "passed": 39689, "failed": 0 }, "environment": { "os": "GitHub Actions ubuntu-latest (non-WSL); same run green on the windows-latest and macos-latest conformance lanes. Failing-first check: Linux container (node:24-bookworm)", "node": "v24.21.0" }, "output_excerpt": "7fe59cf8 is bfa3c49b merged with next f3f3a182; the merge touches none of this PR's files. Run 42143668096 on 7fe59cf8: every job green (3 full ubuntu shards, targeted ubuntu lane, lint-tests, plugin-validate, QA loop walk, coverage gate, 3 windows + 1 macOS conformance lanes, Required tests). Summed over the full ubuntu shards: # tests 39795 / # pass 39689 / # fail 0 / # skipped 105 / # todo 1. tests/gsd-secret-read-guard.test.cjs locally on this head: 669/669; with next f3f3a182's hook swapped in: 651/669 (18 fail). eslint clean on both files." }Checklist
Fixes #NNNconfirmed-buglabel.changeset/fragment addedBreaking changes
None. The only verdict that loosens is
sudo -c <class> <non-reading command> <secret>(and the-aforms), which now matches the bare command's verdict.